Conversation
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
|
🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 53a84b6 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F148831%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
ZeroIntensity
left a comment
There was a problem hiding this comment.
The C code and the corresponding documentation look good to me. I didn't look very closely at the tests.
| assert(PyFunction_Check(func)); | ||
| PyObject *r = PyFunction_GetModule((PyObject *)func); | ||
| if (!r) { | ||
| Py_RETURN_NONE; |
There was a problem hiding this comment.
Do we need a PyErr_Clear() here? (since if error occurs, usually we will return NULL)
There was a problem hiding this comment.
@ZeroIntensity asked me to remove it (#148831 (comment)) because there is no scenario where this function can return NULL without an exception set.
There was a problem hiding this comment.
Yeah, PyFunction_GetModule can only ever raise if the input argument is not a function, which we know will never be true here. We could add an assert(!PyErr_Occurred()) if you'd like.
There was a problem hiding this comment.
+1 for assert(!PyErr_Occurred())
| *module_name* should be the name of an importable module if the sentinel | ||
| should support pickling. |
There was a problem hiding this comment.
| *module_name* should be the name of an importable module if the sentinel | |
| should support pickling. | |
| For pickling to work, *module_name* must be the name of an importable | |
| module, and the sentinel must be accessible from that module under a | |
| path matching *name*. Pickle treats *name* as a global variable name | |
| in *module_name* (see :meth:`object.__reduce__`). |
There was a problem hiding this comment.
Adding this would help people use pickle properly. People need to assign this sentinel instance to that module before pickle.dump.
| Sentinels importable from their defining module by name preserve their | ||
| identity when pickled and unpickled. Sentinels that are not importable by | ||
| module and name are not picklable. |
There was a problem hiding this comment.
| Sentinels importable from their defining module by name preserve their | |
| identity when pickled and unpickled. Sentinels that are not importable by | |
| module and name are not picklable. | |
| Sentinels are pickled using the standard mechanism for named global | |
| objects. :meth:`sentinel.__reduce__` returns :attr:`~sentinel.__name__`, | |
| which pickle interprets as the name of a global variable in | |
| :attr:`~sentinel.__module__`. Sentinels that are not accessible from their | |
| module under that name are not picklable. |
There was a problem hiding this comment.
Thanks, I agree we should expand more but feel it might be a bit much to say exactly how __reduce__ is implemented. I'll get back to this tonight and push some changes.
|
Buildbot run: A few failures look unrelated But I also see https://buildbot.python.org/#/builders/1865/builds/54 Which is a bit mysterious, is this looking at the builtins of another build? |
Yes. We need to patch this test. It tests pickle compatiblity acrosses python versions. |
diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py
index 6366f12257..c2018c9785 100644
--- a/Lib/test/pickletester.py
+++ b/Lib/test/pickletester.py
@@ -3244,6 +3244,7 @@ def test_builtin_types(self):
'BuiltinImporter': (3, 3),
'str': (3, 4), # not interoperable with Python < 3.4
'frozendict': (3, 15),
+ 'sentinel': (3, 15),
}
for t in builtins.__dict__.values():
if isinstance(t, type) and not issubclass(t, BaseException): |
I opened #148967 to track it. |
Replaces JelleZijlstra#4. See python/steering-council#258.
📚 Documentation preview 📚: https://cpython-previews--148831.org.readthedocs.build/